nonblocking check of pollables when reactor is busy #78
Conversation
| let mut future_group = futures_concurrency::future::FutureGroup::< | ||
| Pin<Box<dyn std::future::Future<Output = bool>>>, | ||
| >::new(); | ||
| future_group.insert(Box::pin(cpu_heavy)); | ||
| future_group.insert(Box::pin(timeout)); | ||
| let result = futures_lite::StreamExt::next(&mut future_group).await; |
There was a problem hiding this comment.
I'm surprised this doesn't work here? Afaict this is just race-semantics?
| let mut future_group = futures_concurrency::future::FutureGroup::< | |
| Pin<Box<dyn std::future::Future<Output = bool>>>, | |
| >::new(); | |
| future_group.insert(Box::pin(cpu_heavy)); | |
| future_group.insert(Box::pin(timeout)); | |
| let result = futures_lite::StreamExt::next(&mut future_group).await; | |
| let result = (cpu_heavy, timeout).race().await; |
Assuming futures_concurrency::prelude::*; is imported of course.
There was a problem hiding this comment.
I just tried this change, and it doesn't actually trigger the broken behavior that the existing test case does. I don't actually understand why, though.
| static READY_POLLABLE: LazyLock<Pollable> = | ||
| LazyLock::new(|| wasi::clocks::monotonic_clock::subscribe_duration(0)); |
There was a problem hiding this comment.
Ha, it feels like we're almost missing a built-in for this in the component model?
cc/ @lukewagner check this out
There was a problem hiding this comment.
My understanding is that there is a non-blocking mechanism to check if a waitable set contains any ready waitables in p3, so this wont be an issue there.
There was a problem hiding this comment.
Yep, that's right @pchickey: calling waitable-set.poll or returning the POLL code from a callback is non-blocking.
src/runtime/reactor.rs
Outdated
| // If no wakers are pending on pollables, there is no work to be done | ||
| // here: | ||
| if reactor.wakers.is_empty() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This one caught me off guard a little. I assume it's correct, but I'm not sure why it's correct. Can you elaborate a little more in the comment?
On line 135 we panic if the list of pollables is empty, but apparently having an empty list of wakers is not a problem. Saying a little more about that would be helpful to me at least.
There was a problem hiding this comment.
This is a good catch, this would have subtly introduced bad behavior in the blocking case, so I added a commit that changes how this is implemented. In the nonblocking case (i.e. the root future is pending but has been woken), we want to skip these calculations and poll call if it cannot wake any other futures. In the blocking case, we want to panic with the most informative message possible, rather than rely on a debug assert, or let wasi trap and the user have to work out why.
as provided by @SilverMira in #70, and tracked in #73 Co-authored-by: SilverMira <[email protected]>
sharing most of the implementation with block_on_pollables
in the nonblocking case, we can skip work. in the blocking case, we need to panic. In the absence of a panic, either the debug assert in block_on_pollables will go off, or the wasi poll() will trap.
Fixes #73.
Add test provided by @SilverMira in #70, tracked in #73.
Add a
nonblock_check_pollablesfunction to the reactor, for use in the "busy case" of the block_on loop. Factor outcheck_pollables, the common part ofblock_on_pollablesandnonblock_check_pollables.